Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pam_gnupg: init at fbd75b7 #77002

Closed
wants to merge 3 commits into from
Closed

pam_gnupg: init at fbd75b7 #77002

wants to merge 3 commits into from

Conversation

ahesford
Copy link

@ahesford ahesford commented Jan 6, 2020

Motivation for this change

The pam_gnupg module allows GPG keys to be unlocked during user authentication and session creation in PAM. This request adds a new package for pam_gnupg to nix, and modifies the PAM module in NixOS to add optional support for pam_gnupg to the system PAM configuration.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @prusnak

{ stdenv, fetchgit, autoreconfHook, gnupg, pam } :

stdenv.mkDerivation rec {
name = "pam_gnupg-fbd75b7";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use this instead of name

pname = "pam_gnupg";
version = "unstable-2019-12-06";

@@ -438,6 +441,8 @@ let
"session optional ${pkgs.otpw}/lib/security/pam_otpw.so"}
${optionalString cfg.startSession
"session optional ${pkgs.systemd}/lib/security/pam_systemd.so"}
${optionalString config.security.pam.enableGnupg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you are mixing tabs and spaces here because the columns do not align.

|| cfg.pamMount
|| cfg.enableKwallet
|| cfg.enableGnomeKeyring
|| cfg.googleAuthenticator.enable
|| cfg.duoSecurity.enable)) ''
auth required pam_unix.so ${optionalString cfg.allowNullPassword "nullok"} likeauth
${optionalString config.security.pam.enableGnupg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you are mixing tabs and spaces here because the columns do not align.

@@ -361,12 +361,15 @@ let
# We use try_first_pass the second time to avoid prompting password twice
(optionalString (cfg.unixAuth &&
(config.security.pam.enableEcryptfs
|| config.security.pam.enableGnupg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems you are mixing tabs and spaces here because the columns do not align.

nixos/pam: add support for new pam_gnupg package in pam configs
Replace tabs with spaces and change unstable version numbering
@ahesford
Copy link
Author

ahesford commented Jan 9, 2020

@prusnak Thanks for the feedback; the requested changes have been made.

Comment on lines 20 to 22
configurePhase = ''
./configure --prefix=$out --with-moduledir=$out/lib/security
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace with:

configureFlags = [ "--with-moduledir=$out/lib/security" ];

Copy link
Author

@ahesford ahesford Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried this originally, but the build failed because the shell somehow expanded "$o" to an empty string and made moduledir "ut/lib/security". The configurePhase redefinition didn't cause the same problem. It turns out that using quoted braces around "out" when using configureFlags also solves the problem, so the latest commit adopts the correct form.

Comment on lines 16 to 18
preAutoreconf = ''
mkdir m4
'';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It silenced a warning during the build phase, but I removed it because it doesn't cause the build to fail.

@ahesford
Copy link
Author

I no longer have a NixOS installation and will be unable to respond meaningfully to further requests for changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants